-
Notifications
You must be signed in to change notification settings - Fork 79
feat: expose callback trigger value for collections #622
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: expose callback trigger value for collections #622
Conversation
/** | ||
* The value that triggered the last update | ||
*/ | ||
sourceValue?: OnyxValue<OnyxKey>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it the value or an object of onyx key and value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OnyxValue
represents a value that can be either a single entry or a collection of entries
@MonilBhavsar friendly bump, could you take a look when you have a moment? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Sorry for delay. Could you please provide a usage for this in App repo, or where we're going to utilize this?
My idea is to create a new derived value that stores certain report attributes that does not change often. The goal is to avoid unnecessary recomputations and significantly reduce processing overhead. In this draft PR, I moved the Initial measurements look very promising—this simple optimization already brings noticeable performance improvements. We can expand this further by moving more static or infrequently changing attributes into derived values to scale the gains even more. |
Thanks for explaining. Makes sense 👍 |
Details
This PR introduces exposing the value that triggered the connection callback. It's helpful when callback is doing some operations for each collection item - e.g. when a collection key is a dependency of derived value in E/App and it loops through all items to compute things, having this trigger value allows to make an update for a single item rather than recompute everything from scratch.
Related Issues
GH_LINK
https://expensify.slack.com/archives/C05LX9D6E07/p1741698890119029
Automated Tests
Added unit tests that check if:
sourceValue
whenwaitForCollectionCallback: true
sourceValue
is not available forwaitForCollectionCallback: false
Manual Tests
Opened a draft PR where
getReportName
is moved to derived value and it updates a single item: Expensify/App#58476If you want to make sure we just did a single-item computation, open chrome dev tools -> Performance and start recording before step no 7, download the trace, upload it to https://www.speedscope.app/ and search for
recomputeDerivedValue
. You should findcompute
function forreportAttributes
here, which should be a very tiny update probably with <1ms of execution time.Author Checklist
### Related Issues
section aboveTests
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop